fix!: null handling when using NOT with scalar indices
#5270
+1,702
−598
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Scalar indexes currently treat NULL values as FALSE when evaluating filters, which violates Kleene three-valued logic. This causes bugs like `x != 5` incorrectly including rows where x is NULL. This commit adds the foundation for proper null handling: - Extended RowIdMask with null_list field to track NULL rows - Implemented Kleene logic for NOT, AND, OR operations - Updated SearchResult to carry null row information - Modified all index implementations for backward compatibility The infrastructure is complete but indexes don't yet track nulls. Follow-up commits will implement actual null tracking in BTree and Bitmap indexes. Fixes lance-format#4756 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
wjones127
added
the
critical-fix
Added unit tests to verify Kleene three-valued logic operations work correctly: - RowIdMask AND/OR/NOT operations with nulls - also_block and also_allow preserve null_list - Serialization/deserialization of null_list - Bitmap index returns null_list in SearchResult These tests verify the null tracking infrastructure works correctly at the Rust level. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Adds comprehensive tests to verify that bitmap and btree indexes correctly track and return null row IDs in query results. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The [tool.ruff] section had a duplicate lint key which caused maturin to fail parsing the file. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
After adding null_list to RowIdMask, the serialization now produces 3 elements (block_list, allow_list, null_list) instead of 2. Updated serialize_to_arrow and try_from_arrow to handle 3 elements correctly. This fixes the "all columns in a record batch must have the same length" error when using scalar indexes with null tracking. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The iter_ids() method was only filtering out block_list entries but not null_list entries. This caused nulls to be included in query results when they should be filtered out according to Kleene three-valued logic. Updated iter_ids() to filter out both block_list and null_list entries, ensuring that null values are never returned in iteration. Added test_iter_ids_with_nulls() to verify the fix. Note: The Python integration test still fails, indicating there may be another code path that needs fixing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
wjones127
changed the title
NOT with scalar indices
wjones127
changed the title
NOT with scalar indicesNOT with scalar indices
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Previously, NOT operations on inexact index results (AtMost/AtLeast) would keep the same certainty variant while negating the mask. This was incorrect because: - NOT(AtMost(x)) should return AtLeast(!x), not AtMost(!x) (complement of superset is subset) - NOT(AtLeast(x)) should return AtMost(!x), not AtLeast(!x) (complement of subset is superset) This caused incorrect query results when using inexact indices like bloom filters and zonemaps with NOT operators. Changes: - Fixed NOT implementation to flip certainty variants - Removed unused `map` method that was replaced by explicit match - Added unit tests for certainty flipping - Added integration tests for NOT with nulls 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
BREAKING CHANGE: The
SearchResultstruct returned byScalarIndex::search()now wraps aNullableRowIdSetinstead of aRowIdTreeMap. Scalar indices must now provide the set of row ids where the expression value is null instead of just where it is true. Additionally, theRowIdMaskis now an enum instead of a struct.This PR fixes correctness bugs that show up when (a) running a filter with
NOT, (b) the column you are filtering on contains nulls, and (c) we are using a scalar index (such as btree, or bitmap). Previously, this would give the wrong answer:It should not include
null. The reason it did is that ourRowIdMask(which is output by a scalar index query) was not aware of nulls. So when it processedvalue < 2, it would select just row index0. ThenNOTwould invert that to[1, 2], selecting both[false, null].This PR makes
RowIdMaskaware of nulls. When it processesvalue < 2, it recordsselected: [0]andnulls: [2]. Then, when you invert that and then drop, you getselected: [1], giving the correct final answer of just[5].As part of this, we also refactor RowIdMask to make allow list and deny list mutually exclusive, which simplifies some of the logic.
Fixes #4756